feat: add public profile Open Graph previews#400
Conversation
|
@Harxhit , |
@ShantKhatri please check it . |
ccae586 to
ece9eea
Compare
|
Someone is attempting to deploy a commit to the Prashantkumar Khatri's projects Team on Vercel. A member of the Team first needs to authorize it. |
CI — Checks FailedBackend — FAIL
Mobile — SKIP
Web — PASS
Last updated: |
|
@ShantKhatri , please review it . |
Hi, checks are failing. |
Please allow me some time. |
|
@Harxhit , |
Signed-off-by: amritbej.sh <amritbej750@gmail.com>
|
@Harxhit , |
Harxhit
left a comment
There was a problem hiding this comment.
Review: this branch does not build
The headline issue is that the merge of main into this branch (5852175) was resolved by concatenating both sides of the conflict in apps/backend/src/routes/public.ts instead of merging them. The result does not parse, let alone type-check, so none of the validation listed in the PR description can be passing on the current head.
Reproduced locally on 5852175:
# typecheck
src/routes/public.ts(81,7): error TS1005: 'try' expected.
src/routes/public.ts(136,7): error TS1005: 'try' expected.
src/routes/public.ts(182,7): error TS1005: 'try' expected.
src/routes/public.ts(231,4): error TS1128: Declaration or statement expected.
# lint
src/routes/public.ts 81:6 error Parsing error: 'try' expected
# vitest
The symbol "CACHE_CONTROL_HEADER" has already been declared (public.ts:34:6)
Unexpected "catch" (public.ts:81:6)
-> public.test.ts: 0 tests collected, suite fails to load
The PR note says the build is "blocked by existing unrelated TypeScript issues" — that is not accurate. The failures above are caused by this branch's merge, not by pre-existing problems, and they are syntax errors, not type nits.
Must fix (blocking)
public.ts: redo the merge by hand. Every handler now contains the new code, a duplicated copy of the old code that is unreachable after areturn, and a secondcatchblock — that is what produces the'try' expectederrors at lines 81, 136, 182. Duplicatedimport * as publicService(lines 3 & 9) and duplicatedconst CACHE_CONTROL_HEADER(lines 21 & 34) also need to collapse to one each.- Get
tsc --noEmit,eslint, andvitestgreen and re-run the commands in the PR description before re-requesting review.
Should fix
- The client-side OG meta tags will not be seen by any social scraper. Slack/Twitter/Facebook/Discord/WhatsApp crawlers do not execute JavaScript; they read the initial HTML response. Injecting
og:*/twitter:*via a ReactuseEffect(ProfilePage.tsx) means the rich preview this PR is built for never renders for those bots. The backend image endpoint is fine, but the tags pointing at it need to be server-rendered (SSR / prerender / a meta-injecting middleware on the profile route). As written, the feature does not achieve its stated goal. fetchAvatarBase64SSRF guard is incomplete and unbounded. Requiringhttps://does not prevent SSRF —https://169.254.169.254/...,https://localhost, andhttps://10.0.0.xare all still fetched server-side. There is also no response-size cap, so a malicious avatar URL can stream an arbitrarily large body into memory viaarrayBuffer(). See inline comment.
Nits
- The OG route queries
app.prisma.user.findUniquedirectly, while every other handler in this file goes throughpublicService. Inconsistent; consider moving it behind the service for testability and parity. buildMetaDescriptioncounts platforms viaprofile.links.length, but the image counts_count.platformLinks. These can disagree (links shown vs total connected).
The OG image generation itself (og-image.ts SVG template, XML escaping, hex sanitisation, initials fallback, fire-and-forget cache write, the test coverage) is solid work. The blocker is purely the broken merge — fix that and most of this is mergeable.
|
@Harxhit , |
Summary
Closes #33
Validation
pnpm --filter @devcard/backend exec eslint src/routes/public.ts src/utils/og-image.ts src/__tests__/public.test.tspnpm --filter @devcard/backend exec vitest run src/__tests__/public.test.tspnpm --filter @devcard/web checkNote
pnpm --filter @devcard/backend buildis currently blocked by existing unrelated TypeScript issues in backend test files.